lib/repo: cleanup_tmpdir should be executed after releasing lock file

Here's a subtle bug in abort_transaction():
One of the policies of cleaning up is to skip the current boot's staging
directory. The responsible function for this is cleanup_tmpdir() which tries
to lock each of the tmpdir before deleting it. When it comes to the current
boot's staging dir, it tries to lock the directory(again!) but fails as there
is already a lockfile present. Just because the current boot's staging dir was
meant to be skipped, the bug never surfaced up and wasn't catastrohpic.

if (!_ostree_repo_try_lock_tmpdir (dfd, path, &lockfile, &did_lock, error))
  return FALSE;
if (!did_lock)
  return TRUE; /* Note early return */
...
if (g_str_has_prefix (path, self->stagedir_prefix))
  return TRUE; /* Note early return */

The actual check for skipping staging dir for current boot was never reached
because the function returned at did_lock failure.

Therefore, execute cleanup_tmpdir() after releasing the lockfile in
abort_transaction() so that cleanup_tmpdir gets a chance to lock current boot's
staging directory and succeed.

Closes: #1602
Approved by: jlebon

diff --git a/src/libostree/ostree-repo-commit.c b/src/libostree/ostree-repo-commit.c
index 2516ba3d5c81ab6ece56159c8926dd75f9b853bf..ce2f4deb56966b2424b2946a1fbad9f13db11bf6 100644 (file)
--- a/src/libostree/ostree-repo-commit.c
+++ b/src/libostree/ostree-repo-commit.c
@@ -2229,10 +2229,6 @@ ostree_repo_abort_transaction (OstreeRepo     *self,
   if (!self->in_transaction)
     return TRUE;
 
-  /* Do not propagate failures from cleanup_tmpdir() immediately, as we want
-   * to clean up the rest of the internal transaction state first. */
-  cleanup_tmpdir (self, cancellable, &cleanup_error);
-
   if (self->loose_object_devino_hash)
     g_hash_table_remove_all (self->loose_object_devino_hash);
 
@@ -2242,6 +2238,10 @@ ostree_repo_abort_transaction (OstreeRepo     *self,
   glnx_tmpdir_unset (&self->commit_stagedir);
   glnx_release_lock_file (&self->commit_stagedir_lock);
 
+  /* Do not propagate failures from cleanup_tmpdir() immediately, as we want
+   * to clean up the rest of the internal transaction state first. */
+  cleanup_tmpdir (self, cancellable, &cleanup_error);
+
   self->in_transaction = FALSE;
 
   if (self->txn_locked)